Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Od fixes-- rick's branch rebased on current master [632f6e1] #39

Closed
wants to merge 62 commits into from
Closed

Od fixes-- rick's branch rebased on current master [632f6e1] #39

wants to merge 62 commits into from

Conversation

leamas
Copy link
Contributor

@leamas leamas commented Sep 29, 2019

PLEASE DON*T MERGE THIS BRANCH (sorry for shouting)

This is Rick's branch od-fixes rebased to current master. I't builds on all platforms. I provide it as some input since the process to rebase #34 has been somewhat painful. Some remarks.

  • There is old code around for MacOS installations like INSTALL(TARGETS ${PACKAGE_NAME} RUNTIME LIBRARY DESTINATION ${CMAKE_:BINARY_DIR}/OpenCPN.app/Contents/PlugIns). This is utterly broken, the ${CMAKE_BINARY_DIR} prefix uses build host paths as installation paths. Just drop the prefix to fix.

  • The plugin artifact names are something like squiddio-pi-0.6.4_debian-10.whatever. Alll these parts are required so that for example the same plugin built for Debian 9 and Debain 10 can co-exist in the same directory. Adding new parts to the name is fine, but please don't remove anything.

  • This is a a rough git rebase -X ours so some changes made by @mauroc are certainly lost.

  • EDIT: The bintray deployment in appveyor.yml is dead code, I missed to remove it. Sorry...

HTH... /alec

leamas and others added 30 commits September 15, 2019 14:56
The standard PluginInstall has a nasty bug which uses the source
path as part of installation path. Remove, and use a simple
OpenCPN.app prefix

The loader expects the apple tarball to have paths like:

  - OpenCPN.app/Contents/PlugIns: .dylib plugin file, binaries
    and helper libs.
  - OpenCPN.app/Contents/Resources: .lproj gettext translations
    directories.

The prefix is discarded and could be anything.

Let "make package" build both the traditional installer and the
new installation tarball. Remove references to other formats,
notably rpm.
Some places including bintray uses a common namespace for all
plugins. Make sure the plugin filename is unique by including
both source and target name and version.
We are using deprecated and finally failing stuff.
See: OpenCPN/OpenCPN#1452
Updating the key is error-prone and requires the secret key. It
also requires the travis command line utility. Using that and
the key I was finally able to update .travis.yml using

travis encrypt -x -a deploy.key -r leamas/squiddio_pi  <my key>

-x: Overwrite existing key
-r: Name of repo

Note that the repo is part of the key so after forking a new,
encrypted key must be generated.
The travis bintray integration seems just unstable. Use a raw
access to the REST API in a shellscript instead.
... and move tarball location to cloudsmith
Installable plugin generation
plugin.xml.in: Fix copy-paste error.
@rgleason rgleason mentioned this pull request Sep 29, 2019
@mauroc
Copy link
Owner

mauroc commented Sep 29, 2019

PLEASE DON*T MERGE THIS BRANCH

in other words, I should not merge the pull request with my master? What exactly am I expected to do? Sorry I jumped into this a little late so I need some handholding lest I cause everyone more work (including myself)

some changes made by @mauroc are certainly lost.

All the changes I made in the last 3-4 days were about .travis.yml and appveyor.yml, so if the ones in the PR are functional (other than substituting my own creds for the 2 services), no big loss. Let's call it a learning opportunity for me

@leamas
Copy link
Contributor Author

leamas commented Sep 30, 2019

What exactly am I expected to do?

You are the upstream maintainer here, so in the end it's your call. That said, I am providing this as some help to Rick who seems to have a hard time rebasing his changes on the new tip. Sorry for using your repo as message service, but I see no other way to achieve this.

So, for now I suggest that you just let this PR sit here, waiting for Rick to come up with the real stuff.

@rgleason
Copy link
Contributor

rgleason commented Oct 1, 2019

Leamas,
"PLEASE DON*T MERGE THIS BRANCH (sorry for shouting)"
-Thanks.

@rgleason
Copy link
Contributor

rgleason commented Oct 8, 2019

Alec,
Is it true that all files under ci/ are independent from cmake files?
Also it looks to me like .circleci/config.yml is independent from cmake files too?

If that is true, that leaves
CMakelists.txt, PluginConfigure.cmake, PluginInstall.cmake and PluginPackage.cmake
squiddio_pi.cpp <---very close to identical
squiddio_pi.h <---identical
icons.cpp <--change is in mauro master
appveyor.yml
.travis.yml
to deconflict.

@leamas
Copy link
Contributor Author

leamas commented Oct 16, 2019

So, I definitely don't want to rebase this once again. Closing

@leamas leamas closed this Oct 16, 2019
@rgleason
Copy link
Contributor

rgleason commented Oct 17, 2019

FACT: Alec, you never rebased! You erased history and reverted Mauro's repository back to your original commit, and then created a new commit using your original commit. No rebase needed.

---Yes, you did rebase for this PR that was not used.

@leamas leamas deleted the od_fixes branch October 17, 2019 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants